Skip to content

fix(request-response): don't keep duplicate addresses #4700

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mergify merged 6 commits into from
Oct 25, 2023
Merged

fix(request-response): don't keep duplicate addresses #4700

mergify merged 6 commits into from
Oct 25, 2023

Conversation

@b-zee
Copy link
Contributor

@b-zee b-zee commented Oct 20, 2023 •
edited by thomaseizinger
Loading

Description

Resolves: #4699.

Notes & open questions

In kad, there is a special Addresses struct that might be worth reusing for this functionality.

I will make further CHANGELOG edits etc if this fix is thought to be sensible.

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@b-zee b-zee mentioned this pull request Oct 20, 2023
Closed
@joshuef
Copy link
Contributor

joshuef commented Oct 20, 2023

Nice one. This looks to have been a wee mem leak for us. We've been doing add_server on every Identify event and been leaking decent chunks of memory over time. 💪

joshuef
joshuef reviewed Oct 21, 2023
View reviewed changes
mxinden
mxinden reviewed Oct 21, 2023
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about using a HashSet instead?

modified   protocols/request-response/src/lib.rs
@@ -337,7 +337,7 @@ where
     /// reachable addresses, if any.
     connected: HashMap<PeerId, SmallVec<[Connection; 2]>>,
     /// Externally managed addresses via `add_address` and `remove_address`.
-    addresses: HashMap<PeerId, SmallVec<[Multiaddr; 6]>>,
+    addresses: HashMap<PeerId, HashSet<Multiaddr>>,
     /// Requests that have not yet been sent and are waiting for a connection
     /// to be established.
     pending_outbound_requests: HashMap<PeerId, SmallVec<[RequestProtocol<TCodec>; 10]>>,

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Oct 22, 2023

How about using a HashSet instead?

modified   protocols/request-response/src/lib.rs
@@ -337,7 +337,7 @@ where
     /// reachable addresses, if any.
     connected: HashMap<PeerId, SmallVec<[Connection; 2]>>,
     /// Externally managed addresses via `add_address` and `remove_address`.
-    addresses: HashMap<PeerId, SmallVec<[Multiaddr; 6]>>,
+    addresses: HashMap<PeerId, HashSet<Multiaddr>>,
     /// Requests that have not yet been sent and are waiting for a connection
     /// to be established.
     pending_outbound_requests: HashMap<PeerId, SmallVec<[RequestProtocol<TCodec>; 10]>>,

+1 for using a HashSet. Long-term, we should solve this by adding a PeerStore behaviour: #4103.

b-zee added 2 commits October 23, 2023 14:39
@b-zee
keep peers in HashSet instead of SmallVec
98355b6 
This prevents us from adding duplicates into the list of peer addresses we have.
@b-zee
update request-response CHANGELOG.md for libp2p#4700
0c4b667
@thomaseizinger thomaseizinger changed the title fix: deduplicate autonat add_server addresses fix(request-response): don't keep duplicate addresses Oct 23, 2023
@thomaseizinger
Copy link
Contributor

thomaseizinger commented Oct 23, 2023

Thanks! Can you address the clippy failure?

@b-zee
Copy link
Contributor Author

b-zee commented Oct 24, 2023

Fixed the clippy lint.

Is v0.53.0 set to be released soon? Or, possibly, is there a patch release that this could part of soon? Thanks for all the work!

@thomaseizinger
Copy link
Contributor

thomaseizinger commented Oct 24, 2023

Fixed the clippy lint.

Is v0.53.0 set to be released soon? Or, possibly, is there a patch release that this could part of soon? Thanks for all the work!

If you backport it to the v0.52 branch, I can cut a patch release :)

@mergify mergify bot merged commit 7305234 into libp2p:master Oct 25, 2023
@b-zee b-zee mentioned this pull request Oct 25, 2023
Merged
@b-zee b-zee deleted the fix-autonat-add-server-deduplicate branch October 26, 2023 13:58
mxinden pushed a commit that referenced this pull request Oct 26, 2023
@b-zee
fix(request-response): don't keep duplicate addresses
469bb8b 
Backport of #4700 on `v0.52` branch.

- fix: deduplicate autonat add_server addresses
- keep peers in HashSet instead of SmallVec
- change (&HashSet)::into_iter to equivalent iter

Pull-Request: #4724.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mxinden mxinden mxinden left review comments

@thomaseizinger thomaseizinger thomaseizinger approved these changes

+1 more reviewer

@joshuef joshuef joshuef left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

send-it

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AutoNAT servers added are not deduplicated

4 participants

@b-zee @joshuef @thomaseizinger @mxinden